Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix tooltip component positioning and add it to storybook #1031

Merged
merged 2 commits into from
Oct 4, 2023

Conversation

Traxmaxx
Copy link
Contributor

@Traxmaxx Traxmaxx commented Oct 4, 2023

Problem

The tooltip is broken in a way that for unknown reasons classNames is not overriding classes properly. Taking the following component:

function Tooltip({
  children,
  top = 'md',
  align = 'left',
  width = 'md'
}: TooltipProps) {
  console.log(top, 'top');
  return (
    <div
      role="tooltip"
      className={classNames(
        'absolute left-6 top-24 z-[1000] hidden animate-fade-in-up items-center rounded-lg bg-black-900 px-4 py-2 text-xs font-medium text-black-200 opacity-0 peer-hover:flex',
        { 'top-0': top === 'xs' },
        { 'top-[3rem]': top === 'sm' },
        { 'left-auto right-0': align === 'right' },
        { 'w-72': width === 'lg' }
      )}
    >
      {children}
    </div>
  );
}

and using it as follows

<div className="relative">
  <WarningIcon className="peer" height="16" width="16" />
  <Tooltip top="xs" align="left" width="lg">
    Only AWS resources are currently supported on the explorer.
  </Tooltip>
</div>

renders the following HTML

Screenshot 2023-10-04 at 12 22 29

and as you can see there is still the top-24 being applied and the top-0 is doing nothing.
Logging the top property shows that it is indeed passed in correctly:

Screenshot 2023-10-04 at 12 22 37

However the rendering is completely off and not working as expected

Screenshot 2023-10-04 at 13 23 27

Solution

I fixed the class composition and added some more properties to also allow it to be positioned from the bottom. I kept the current positioning syntax to not break existing tooltips throughout Komiser.

I also added a Storybook component since it was not super clear from the beginning on how to actually use the component. I hope that the Story makes this a bit more clear that you need to wrap the Tooltip inside a relative positioned container and that you need to have an peer element which works as a trigger for the tooltip to show!

Changes Made

  • Fix the Tooltip component
  • Add more options like bottom to allow all varieties of positioning
  • Add Story for documentation purposes
  • Use the new options in the dependency graph to have a properly positioned Tooltip

How to Test

Either check out the branch or check Storybook

Screenshots

Screenshot 2023-10-04 at 13 11 11
Screenshot 2023-10-04 at 13 12 24

Notes

None

Checklist

  • Code follows the contributing guidelines
  • Changes have been thoroughly tested
  • Documentation has been updated, if necessary
  • Any dependencies have been added to the project, if necessary

Reviewers

To be reviewed by code owners

@mlabouardy mlabouardy merged commit ad517d8 into develop Oct 4, 2023
2 checks passed
@mlabouardy mlabouardy deleted the fix-tooltip branch October 4, 2023 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants